Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix incorrect usage of add_handled_dependencies #10270

Merged
merged 10 commits into from
Jul 23, 2024

Conversation

jakecoffman
Copy link
Member

@jakecoffman jakecoffman commented Jul 22, 2024

What are you trying to accomplish?

Finally got to the root cause of the issue I've been hunting, previous PRs for additional context:

The issue was caused by DependencySnapshot's methods needing current_directory being set beforehand, but several places we were calling add_handled_dependencies it wasn't set yet.

To fix that, I made a helper mark_group_handled which is appropriate to call in those places. It's also a good place to start using directory in the existing group PR data, but I'll save that for another PR.

This has also fixed a bad test vu-group-multidir-semver.txt had an incorrect second assertion. There are 4 dependencies across 2 directories, with a major and minor group all 4 should be updated in 2 PRs. This is fixed!

Anything you want to highlight for special attention from reviewers?

How will you know you've accomplished your goal?

I will manually test against our repo that was getting individual PRs when it should not have.

Checklist

  • I have run the complete test suite to ensure all tests and linters pass.
  • I have thoroughly tested my code changes to ensure they work as expected, including adding additional tests for new functionality.
  • I have written clear and descriptive commit messages.
  • I have provided a detailed description of the changes in the pull request, including the problem it addresses, how it fixes the problem, and any relevant details about the implementation.
  • I have ensured that the code is well-documented and easy to understand.

@jakecoffman jakecoffman force-pushed the handle-multidir-dep-updates-correctly branch from 5d9d9c1 to 34fdbe1 Compare July 22, 2024 19:44
@jakecoffman jakecoffman changed the title handle multi-dir updates existing PRs correctly refactor DependencySnapshot to fix a bug with add_handled_directories Jul 22, 2024
@jakecoffman jakecoffman changed the title refactor DependencySnapshot to fix a bug with add_handled_directories refactor DependencySnapshot to fix a bug with add_handled_dependencies Jul 22, 2024
@jakecoffman jakecoffman changed the title refactor DependencySnapshot to fix a bug with add_handled_dependencies fix incorrect usage of add_handled_dependencies Jul 23, 2024
@jakecoffman jakecoffman marked this pull request as ready for review July 23, 2024 13:30
@jakecoffman jakecoffman requested a review from a team as a code owner July 23, 2024 13:30
@jakecoffman jakecoffman merged commit d045d42 into main Jul 23, 2024
122 checks passed
@jakecoffman jakecoffman deleted the handle-multidir-dep-updates-correctly branch July 23, 2024 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants